-
Notifications
You must be signed in to change notification settings - Fork 716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Promise (Lazy) Support + Global onActivation/onDeactivation #1132
Conversation
Hi, nice work! This looks interesting! I am shopping around for a DI system that handles async instantiation. If I'm not mistaken, one should basically only create async providers(bind(x).toDynamicValue) for i.e. independent units ? Is that generally the way your apps have structured themselves ? I'm new to How has been the reception to your patch so far ? How close is this to being "finished" It's not apparent to me immediately what "onActivation" means. |
@sublimator my issue with loadAsync, is it assumes you want to bootstrap all of your async dependencies in one go, OR, attempt to break up your async dependencies in various independent modules and/or containers and load as you need. for example, you in theory could do:
in the above, you would loadAsync root + config and i/o on start of a web app. while that works great for that, now lets assume you want to transactions per web request, you can either use the "request scope" pattern inside Inversify or create a child container for each request and handle isolation that way. now lets say, you have lots of I/O (in my case, I use about 10-20 different AWS products/open source systems), what realistic approach can you take with chunking up all your async dependencies? more so, what happens when all of your request endpoints may only use 2-3 at any given time and can vary a lot? you end up having a lot of "sub containers"/module for every single async scenario you need, which gets sloppy and hard to maintain (and will likely break fundamental things like pooling across the entire process). I shouldn't have to create custom containers for every web request that has unique I/O requirements. add ANOTHER layer on top of that, where you have tests that can have varying degrees of requirements. i personally believe, i should be able to open an IDE, right click > run test, and everything needed to bootstrap THAT test, is complete. I have built my own framework (which I will open source soon), that allows me to have access to a unique container per test, while sharing a root container (for connection pooling), allowing me to run tests fast and in parallel (by utilizing transactions, virtual partioning, etc). add ANOTHER layer :) where i have a monolith project that has both a Web layer, CLI layer (for occasional debug/one-off needs), Job layer (batch/stream processing/cron) and every Job/CLI has unique I/O requirements. I need to create containers for each of those as well? that is a lot of complication/overthinking for just wanting an auto-built graph :) onActivation/onDeactivation is powerful! here are many ways i use it:
i can forsee people creating all types of neat middleware for bootstrapping functionality into existing containers, without having to pollute your codebase with secondary concerns. in my ideal world, a project works like this:
there is no need to worry about environment state, the async container system can boostrap for you (if you so choose). if you prefer the simple, monolith/waterfall approach to async dependencies because you only have a handful, this may seem like overkill. but when your test suite is 1K+ and your CI builds go 30mins+ on tests alone, you come to appreciate async'ness. as to this patch, it has been a ghost town from the project owners since my first push earlier this year. i have seen some commit activity from @dcavanagh but no one has put in their two cents. i may just create a fork of project or roll it into my framework if there is no movement. |
Thanks the detailed explanation. Will reply tomorrow. 9pm here :)
|
For now: “Totally makes sense to me”
Fine grained (== perf etc) rather than load ALL ur async leaves ...
|
Hey, well I hope they take a look at it and least give some rationale for why they won't accept (if that's the case). I think your reasons for wanting async are all quite compelling and I think once people start using it, it would be "why haven't we had this sooner??". Soooo* much JavaScript code is async ... Is it possible the maintainers are just busy? There are 17 pull requests and 152 issues competing for maintainer attention. |
src/resolution/instantiation.ts
Outdated
const constructorInjectionsRequests = childRequests.filter((childRequest: interfaces.Request) => | ||
(childRequest.target !== null && childRequest.target.type === TargetTypeEnum.ConstructorArgument)); | ||
|
||
const constructorInjections = constructorInjectionsRequests.map(resolveRequest); | ||
|
||
if (constructorInjections.some((ci) => ci instanceof Promise)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't that massive a change huh ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope :) i hacked up half the project the first time around trying to get this to work, and then realized it would be as simple as promise detection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ponder points:
- 3rd party Promise libs. Do they generally just inherit from Promise ? I never use them, but I guess some people (still?) do.
- What happens when you have "concurrent" async requests going on at once for a given container (Say a parent container, and per http request child containers accessing the parent container for 'shared' deps). Is that supported ? Does this do the right thing in terms of honoring the scopes etc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
i'd assume if anyone is using a 3rd party lib, they are probably doing so as a polyfill, which would override window.Promise, and this should still be true.
-
from what I understand of my latest implementation, if there is a race condition with async, it would also exist with non-async (eg: dynamic values, etc). the same semantics of how things are looked up in parent containers and/or cached on bindings ring true with promises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks :) So the promises themselves are cached if I am understanding correctly?
Yep
…Sent from my iPhone
On Sep 7, 2019, at 3:48 AM, Nicholas Dudfield ***@***.***> wrote:
@sublimator commented on this pull request.
In src/resolution/instantiation.ts:
> const constructorInjectionsRequests = childRequests.filter((childRequest: interfaces.Request) =>
(childRequest.target !== null && childRequest.target.type === TargetTypeEnum.ConstructorArgument));
const constructorInjections = constructorInjectionsRequests.map(resolveRequest);
+ if (constructorInjections.some((ci) => ci instanceof Promise)) {
I see, thanks :) So the promises themselves are cached if I am understanding correctly?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I don't think that you should be using instanceof Promise. How do I tell if an object is a promise. promisfied.ts
|
@tonyhallett while that is true, i don't see why anyone would explicitly switch between native/non-native and choose not to use polyfills. i suspect babel and most manual implementations do something like this:
|
@tonyhallett just got bit by this bug on lambda... looks like thenable/A+ check it is :) |
Not at all ! I will tie up these loose ends and let's get this feature in ! I will then have a pull request that removes scope management from the resolver. This facilitates a root request scope #1143 as well as custom scopes. If you could review that if you have time that would be great. |
@notaphplover when you have time let's get this in ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent job! Thank you so much @tonyhallett , I think we should avoid to change one interface but everything else is great! Would you agree if I undo the change ar .createChild()
?
I think that it is highly unlikely to cause any issues but yes let's leave it for another time. Please go ahead and revert the change and, dare I say it, merge it in ! |
I'm writing @dcavanagh now, he is the one who should merge this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@notaphplover
We should really sort this out.
@@ -79,7 +81,7 @@ const _resolveRequest = (requestScope: interfaces.RequestScope) => | |||
result = binding.cache; | |||
binding.activated = true; | |||
} else if (binding.type === BindingTypeEnum.Constructor) { | |||
result = binding.implementationType; | |||
result = binding.implementationType as unknown as T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem here due to the typing
public toConstructor<T2>(constructor: interfaces.Newable<T2>): interfaces.BindingWhenOnSyntax<T> {
this._binding.implementationType = constructor as any;
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add breaking changes to solve it. Do you have anything in mind?
We have plans to change typings, but I think we should accept they are not perfect :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have anything in mind?
It will need some thought for sure !
I think we should add breaking changes to solve it
That will probably be necessary.
It's only a problem as resolveRequest is typed to return undefined | T | Promise<T> | (T | Promise<T>
All other returns are typed as any !
Perhaps better to untype the return for now.
Problems still remain but they were there before this feature.
I will happily look at the typings at a later date.
What I would like to do is to remove all of this code !
if (binding.type === BindingTypeEnum.ConstantValue) {
result = binding.cache;
binding.activated = true;
} else if (binding.type === BindingTypeEnum.Function) {
result = binding.cache;
binding.activated = true;
} else if (binding.type === BindingTypeEnum.Constructor) {
result = binding.implementationType as unknown as T;
} else if (binding.type === BindingTypeEnum.DynamicValue && binding.dynamicValue !== null) {
result = invokeFactory(
"toDynamicValue",
binding.serviceIdentifier,
() => (binding.dynamicValue as (context: interfaces.Context) => any)(request.parentContext)
);
} else if (binding.type === BindingTypeEnum.Factory && binding.factory !== null) {
result = invokeFactory(
"toFactory",
binding.serviceIdentifier,
() => (binding.factory as interfaces.FactoryCreator<any>)(request.parentContext)
);
} else if (binding.type === BindingTypeEnum.Provider && binding.provider !== null) {
result = invokeFactory(
"toProvider",
binding.serviceIdentifier,
() => (binding.provider as interfaces.Provider<any>)(request.parentContext)
);
} else if (binding.type === BindingTypeEnum.Instance && binding.implementationType !== null) {
result = resolveInstance(
binding,
binding.implementationType,
childRequests,
_resolveRequest(requestScope)
);
} else {
// The user probably created a binding but didn't finish it
// e.g. container.bind<T>("Something"); missing BindingToSyntax
const serviceIdentifier = getServiceIdentifierAsString(request.serviceIdentifier);
throw new Error(`${ERROR_MSGS.INVALID_BINDING_TYPE} ${serviceIdentifier}`);
}
and instead binding.provideValue(context)
and have binding as abstract with a derivation for each of the BindingToSyntax distinguishable methods.
( requestScope not necessary with the scope feature I want to add when this goes through )
Perhaps this would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well T is supposed to be the result's type, I understand your feeling, I really want to improve that code's block, but (T | Promise | (T|Promise)[]) is a valid returning type. A constructor binding is suposed to be called with a T extends Newable<unknown>
, considering that (T | Promise | (T|Promise)[]) is a valid return type.
I'm looking forward to improve that code, but I believe achieving changes through small PR is the best way to go. It's easier to revier a small PR, it's easier to solve conflicts so it helps this project to scale. So I prefer to solve this in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss the return type at a later date. I agree another PR for type changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please see #1319
Great, I see this was merged! :) |
Hi! @dcavanagh! Super-great DI library! We are looking forward to use these async dependencies! Are there any chance that it will be published to npm soon? |
@tonyhallett @notaphplover @dcavanagh Cheeky bump :) |
I hope this gets released soon - the documentation on Very happy to be using the existing functionality in the meantime! I know that the life of an OSS dev is fraught with grind and thanklessness, I appreciate everybody's effort so much for this 🙏 |
Up! |
This is a cleaner refactor which combines #1074 and #1086 to allow for users to opt-in to lazy loaded dependencies by using an asynchronous API.
For anyone wanting to play around with this, you can use the NPM release:
@parisholley/inversify-async:1.1.7
Description
New container methods for pulling objects out of the container in an asynchronous fashion while respecting container semantics and error if trying to mix synchronous/asynchronous dependencies.
Implementation
Global onActivation
Binding onActivation
Parent/Child onActivation
Module onActivation
Related Issue
#418
#475
#887
#404
Motivation and Context
Use Case 1: Unit/System Testing
While you technically could create separate containers (with a parent container for common objects) for a unit test environment and system test (database, etc), it adds more code complexity and also requires each system integration be live and working in order to bootstrap the container (this matters, because for instance in the IDE, if I want to run a single test, with a single I/O dependency, I don't have to initiate every connection).
Use Case 2: Common Container Use
Let's say you have a "service" code base that is re-used between a web application, command line interface and batch processing. In the case of the CLI, you have different commands that serve different purpose, eg:
By making I/O dependencies lazy loaded (async), our CLI can wait until runtime to connect to the system that it needs for the task (otherwise, same problem as above, you are forced to connect-to, and have access-to systems that are unrelated to the command at hand).
Use Case 3: Transactions:
For testing (or perhaps a custom request based framework where child containers are created for each web request to implement "Request Scope" similar to spring), it may be useful to have async construct/destroy methods. For example, a web request where we want to run all service calls in a transaction and commit on at the end (by unbinding the container), or in tests by rolling back database changes for more performant runs.
How Has This Been Tested?
Full suite of tests have been implemented.
Types of changes
Checklist: